-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add support for field_behavior options on swagger plugin #1806
add support for field_behavior options on swagger plugin #1806
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really interesting, thanks for the PR! I'd like to understand the consequences of this change, as it may change the expectations of the users. For example:
- Should we add any handling to
protoc-gen-grpc-gateway
of these annotations? What would that mean? - Adding this file as a dependency of the library runs the risk of annoying all current users who do not have
field_behavior.proto
vendored (as it wasn't a reqiurement), is this going to break users that don't vendor this file? From a cursory reading I don't think it should since we're just checking if the extensions are set.
Also, could we update one of the fields in examples/internal/proto/a_bit_of_everything.proto
to add some of these descriptions to show users how they're used?
Thanks!
You could certainly add runtime checking for some of the options - I think it's a reasonable discussion but probably would be a separate issue.
Confirmed that there is no compile time dependency on
Will do.
|
@johanbrandhorst - I think we are all set here. In relation to the original runtime behavior question, I think there are a couple options for anyone who wants automatic runtime validation against the Swagger spec:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, one small nit, but I have a final request - now that we've added this behaviour, lets write a little docs page about how to use it! Our docs pages are in docs/docs/
. We could either add it to "customizing your gateway" (though that page is getting very big) or consider adding a new one for customizing the swagger output (we should probably have a page for this anyway!).
Thanks for your patience!
I'll update the docs sometime in the next few days. |
Codecov Report
@@ Coverage Diff @@
## master #1806 +/- ##
==========================================
+ Coverage 57.32% 57.39% +0.06%
==========================================
Files 34 34
Lines 3616 3652 +36
==========================================
+ Hits 2073 2096 +23
- Misses 1282 1290 +8
- Partials 261 266 +5
Continue to review full report at Codecov.
|
80a6092
to
82d080a
Compare
@johanbrandhorst I've opened another PR (#1822) to cover the docs piece since the changes were a bit beyond the scope of just field behavior support. Figured we could get this merged and iterate on any docs changes there. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, SGTM!
Thanks a ton for your contribution! |
References to other Issues or PRs
There is a discussion in #669 about adding support for
x-nullable
usingFieldBehavior.OPTIONAL
. While I haven't implemented that behavior here yet, it should be relatively trivial to add.Brief description of what is fixed or changed
This adds support for using
google.api.field_behavior
options to specify whether fields are required or output only. The syntax is a little nicer thangrpc.gateway.protoc_gen_swagger.options.openapiv2_schema
because you can define the values on the individual fields as opposed to the message level.